Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[arithmetic_side_effects] Fix #11393 #11395

Merged
merged 2 commits into from
Aug 25, 2023
Merged

Conversation

c410-f3r
Copy link
Contributor

Fix #11393

changelog: [`arithmetic_side_effects`]: Detect division by zero for `Wrapping` and `Saturating`

@rustbot
Copy link
Collaborator

rustbot commented Aug 24, 2023

r? @Manishearth

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 24, 2023
@Manishearth
Copy link
Member

@bors r+

thanks!

@bors
Copy link
Contributor

bors commented Aug 25, 2023

📌 Commit d802ab2 has been approved by Manishearth

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 25, 2023

⌛ Testing commit d802ab2 with merge 706c48b...

@bors
Copy link
Contributor

bors commented Aug 25, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing 706c48b to master...

@bors bors merged commit 706c48b into rust-lang:master Aug 25, 2023
4 checks passed
@guilliamxavier
Copy link

guilliamxavier commented Aug 25, 2023

Sorry for being late, thanks for the fixes but I have several remarks/questions:

  • The code and the tests in the two "arithmetic_side_effects.rs" files look like this intended to also fix [arithmetic_side_effects] Division by NonZeroU* cannot panic #11392?
  • Yet "arithmetic_side_effects.stderr" looks like expecting the current, unfixed behavior (for both issues)? (probably related to the following points)
  • The is_non_zero pattern is missing the *size variants
  • Actually "If the RHS is NonZero*, then division or module by zero will never occur" (and the corresponding code) is too broad, only NonZeroU* (unsigned) are safe (actually the signed NonZeroI* don't even impl Div/Rem, but if they did then e.g. isize::MIN by NonZeroIsize(-1) would panic)
  • "For `Saturation` or `Wrapping` (RHS), all but division and module are allowed" seems right, but "For `Saturation` or `Wrapping` (LHS), everything is allowed" is not: e.g. wrapping_x / wrapping_zero or wrapping_x /= primitive_zero (hopefully the naming is clear) will panic; maybe these two LHS and RHS checks should be combined into a single one?
  • [suggestion] is_div_or_rem could be moved up before the first check and used in it too?

@c410-f3r
Copy link
Contributor Author

This PR was meant to fix both #11392 and #11393 but the lack of the NonZeroUsize symbol and the performance cost of "interning" was a deal breaker in my perspective. I created a PR in the rust repository (rust-lang/rust#115177 (comment)) to improve this scenario and soon another PR using the newly introduced symbols will be submitted to finally close #11392.

You are right in regards to other topics and I will try to fix the concerns as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[arithmetic_side_effects] Division by potentially-zero can panic even with Wrapping
5 participants